Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle errors correctly based on suite.options.error and the number of parameters expected by the vow #263

Merged
merged 24 commits into from
May 13, 2014

Conversation

adamstallard
Copy link

Currently, if an error occurs in a topic and a subsequent vow expects 2 or more arguments, the first (error) argument will be set to null and the second will contain the error.

Currently a reference error in a topic will be thrown.

This the expected (fixed) behavior:

Don't throw errors; catch them and handle them as follows: When suite.options.error is set to false or a vow expects two or more arguments, return the error as the first argument and don't report it (let the user handle it); When suite.options.error is set to true (default) and a vow expects zero or one parameters, report the error in the test runner and don't run the vow.

Update:

I fixed another issue (#231) in this pull request: sub-topics will still proceed if the parent topic throws or returns an error.

Update:

Apparently, basic error-handling through vows is also broken(#280)--fixed by this pull request

…f parameters expected by the vow: When suite.options.error is set to false or a vow expects two or more parameters, get the error as the first argument and don't report it; When suite.options.error is set to true and a vow expects zero or one parameters, report the error and do not run the vow.
@JerrySievert
Copy link

can you do a merge so that there's a possibility of the merge occurring without conflicts? if so, i will take a look.

thanks much!

@adamstallard
Copy link
Author

I merged from upstream and resolved the conflicts in lib/vows.js and lib/vows/reporters/spec.js (I chose the upstream over my changes). I ran the tests and they work--except isolate and suppress-stdout are still broken on Windows, node 0.10. (They do work on linux though)

@JerrySievert
Copy link

hm. i seem to have lost push after the move from @cloudhead to @flatiron - @indexzero can you reinstate?

@adamstallard
Copy link
Author

@JerrySievert thanks for looking at this again. I haven't merged from upstream on my fork for 9 months, so let me know if you need me to do that.

@adamstallard
Copy link
Author

@flatiron I'd be willing to be a maintainer of vows as well (of the repo and/or the npm package). I'm pretty familiar with the code and I still use it as my primary js test framework. Also, how is the website maintained? The documentation there is out-of-date.

@adamstallard
Copy link
Author

Looks like the only thing that's changed since I merged last was adding the repository field to package.json #283 , so this should still be good to merge with no conflicts and I won't both to update my fork just for that.

JerrySievert added a commit that referenced this pull request May 13, 2014
Handle errors correctly based on suite.options.error and the number of parameters expected by the vow
@JerrySievert JerrySievert merged commit aa8cd5e into vowsjs:master May 13, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants